-
-
Notifications
You must be signed in to change notification settings - Fork 536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[18.0][ADD] account_payment_base_oca: New base module for OCA/bank-payment #1401
base: 18.0
Are you sure you want to change the base?
Conversation
Why this? Why didn't you answer to my comment in the issue? I don't want data model changes if they are not justified. |
I'm trying to adopt the new datamodel. I'm not saying it's the best choice, but I want to try and see what difficulties I face. |
Then we are going to have 2 forks. I don't want that "datamodel", as it's very bad as explained in the migration issue. |
0920212
to
d819284
Compare
I just pushed a new version of account_payment_base_oca. In this new version, the active field of account.payment.method.line (added by account_payment_base_oca) is set to False by default. So, when a account.payment.method is added, all the account.payment.method.line automatically created by Odoo are inactive. You can active the ones you need and leave the others inactive. |
I sincerely see it as a nonsense to have to look for "similar" things one way and the reverse because of a failed datamodel putting the data at journal level. You can't for example group by this field if you select different lines according the journal but representing the same payment mode. Let's keep our |
d819284
to
7871388
Compare
@pedrobaeza It's also a big temptation for me to keep account.payment.mode. In fact, I'm the one who introduced account.payment.mode in v9 during the Sorrento code sprint (to replace payment.mode that was dropped by Odoo SA with the removal of the module account_payment from the official addons), so it's kind of my baby and I like it. But, in the long list of wired/sub-optimal datamodel of Odoo SA that I would love to avoid/change, this one is not a big deal. I think I found a solution that is acceptable and the balance between the advantage of being compatible with the native datamodel and the drawbacks of using the trick I propose to make the new datamodel work with bank_account_link = 'variable' tend towards the adoption of the new datamodel I think. I'll continue my tests in the coming days to make sure I don't discover any other problems and I'll publish the upper-level modules. |
7871388
to
9da52f0
Compare
# the account.payment but it will select the (inactive) method line | ||
# linked to the chosen journal | ||
# TODO default=False causes problems in tests of the account module | ||
active = fields.Boolean(default=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to set active=False on create only when bank_account_link is variable?
This sounds promising. I also prefer using the native |
I insist: how it would be better to assign at partner level the payment method line linked to a specific journal, seeing all of them in the dropdown (and this active=False trick is something conflicting with the way Odoo expects to work), instead of having an agnostic payment mode? The payment method lines are not new. They are there since ages (v14). The only new thing is that there's a many2one at partner level, but that doesn't change anything. Please don't go with this, or we will have 2 |
Switch back to active True by default
Another reason spotted now for not switching from |
@pedrobaeza I think Alexis is doing a lot of very valuable work here. Let's wait a bit to see where this goes. |
No, if the foundations are incorrect, no more work should be put in here. I announce my intention on continuing with the old stack if you switch, and the model names should be preserved for the existing things according current establishment. |
Sorry I don't understand your arguments, Pedro. Odoo is evolving and it seems normal to me to explore and see if we can adapt. |
The arguments are clear of the cons of switching, so I'm just stating that I would push for the previous approach and this will conflict in the model names, and it's clear that these cons can't be bypassed without doing ugly hacks and standard code overrides. |
…riable_journal_ids
Note that I'm the first one embracing upstream changes when they are suitable for the goal of the modules. I did the refactoring to But in this case, it has no benefit to go with a simple wrong m2o extra field that they have added in the partner, and that is so wrong in the concept. And I insist that the |
Hello all, I try to understand this part :) Is there a document that list user case for payment_mode use. This can be a good way to check If new or old datamodel can be used and if there is a problem with ? I think à create a document but if you have one, I will try not to start with empty blank sheet :) |
Basically, the payment mode tries to model the payment "instrument" that the customer (or you for vendors) is going to use. Let's say "Wire transfer", "Check", "SEPA Direct Debit", etc. The new Odoo field in standard is just adding a sublevel, like SEPA DD for journal (bank) X, so the side effects are:
That's way the other data model avoids this problem doing an aggregation before, and only "unfolding" to the corresponding bank (journal) on the final end when you do the payment/debit order (although you can constraint to one or several banks from the beginning if you want). This PR tries to "bypass" the side effects doing that you select one of the many "SEPA Direct Debit", and it's changed "on the fly" when doing the payment order. But keeping this so long stable (and simple) extra data model avoids all these problems, and we have also provided other useful tools around it like dynamic text shown in the invoice (for saying in which customer bank account is going to be done the DD, own bank account for wire transfer, etc), that will be lost (or redone) in case of the switch. We can hide totally this other m2o field in the partner view for not having any confusion, at that's all. For me is a total nonsense to even consider doing this switch. |
bank_account_required = fields.Boolean( | ||
related="preferred_payment_method_line_id.payment_method_id.bank_account_required", | ||
) | ||
payment_method_code = fields.Char( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would need a computation script as the table account.move can be heavy on some projects
We need to unblock this for having a complete 18.0, and unless other strong opinions against arise, the natural trend is to keep what we have in previous versions and discuss later any refactoring (if any). We will resume the migration work next week doing the raw migration fine-tuning the UX. |
Work in progress. Expect datamodel changes.